[AAP-52229] Add AuditableModel inheritance to RBAC assignment models #850
Conversation
arrestle
changed the title
arrestle
force-pushed
the
aap-52229-activity-stream
branch
6 times, most recently
from
188637b to
2b2b0e0
Compare
arrestle
force-pushed
the
aap-52229-activity-stream
branch
from
f365373 to
3ee0c76
Compare
- Add conditional AuditableModel inheritance to RoleUserAssignment and RoleTeamAssignment - Create DummyAuditableModel for services without activitystream app - Add comprehensive tests for activity stream functionality and dummy model interface - Exclude object_role field from activity stream logging - Enhance activity entry display for role assignments
arrestle
force-pushed
the
aap-52229-activity-stream
branch
from
535bb88 to
7d5432e
Compare
|
I don't know what's up with checks, |
| if self.content_type and self.content_type.model.lower() in ['roleuserassignment', 'roleteamassignment']: | ||
| operation_text = self.get_operation_display() | ||
| created_by_text = str(self.created_by) if self.created_by else "Unknown" | ||
| return f'[{self.created}] Role assignment {operation_text.lower()} by {created_by_text}' |
There was a problem hiding this comment.
I agree a custom string would make sense because "roleuserassignment" is verbose. But I would question whether we could solve this generally by using _meta.verbose_name.title() instead? Because "Role assignment" drops the user/team designation, which could be useful. This also appears to drop the object_id. I don't think content_type (as a string) was ever useful but it dropped that too.
There was a problem hiding this comment.
You could drop the object_id, but you would want to replace it by like self.content_object.object_id, which yeah, is confusing. That's the target object of the assignment.
There was a problem hiding this comment.
I thought about this and decided that if there were any additional assignments implemented in the future they might not necessarily want to be tracked, that decision should be up the PM and the Developer. This code is here to prevent unwanted side-effects.
note There are lots of ways to handle this, such as an attribute on the class
There was a problem hiding this comment.
My suggestion here is that we try to make AuditableModel just a normal python class, and not a Django abstract model at all. That way, it can be "seen" and imported by thing not even using Django. You could even move to ansible_base.lib and have the activitystream app import it.
There was a problem hiding this comment.
I agree, but since I was asked to move the ticket back into the backlog by John let's take this up on the Thursday Ansible Staff Engineering Weekly Late -- unless you have a better meeting?
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
Description
RoleUserAssignmentandRoleTeamAssignmentmodels in django-ansible-base to inherit fromAuditableModelAuditableModelinheritance enables automatic activity stream logging for all role assignments, providing complete audit trails showing "who assigned what role to whom when"Type of Change
Self-Review Checklist
Testing Instructions
tox -e py -- test_app/tests/rbac/test_rbac_activity_stream.pyExpected Results
Additional Context
aap-gateway test pr#1011